Use `Cow` in `{D,Subd}iagnosticMessage`. #111748

nnethercote · 2023-05-19T01:40:15Z

Each of {D,Subd}iagnosticMessage::{Str,Eager} has a comment:

// FIXME(davidtwco): can a `Cow<'static, str>` be used here?

This commit answers that question in the affirmative. It's not the most compelling change ever, but it might be worth merging.

This requires changing the impl<'a> From<&'a str> impls to impl From<&'static str>, which involves a bunch of knock-on changes that require/result in call sites being a little more precise about exactly what kind of string they use to create errors, and not just &str. This will result in fewer unnecessary allocations, though this will not have any notable perf effects given that these are error paths.

Note that I was lazy within Clippy, using to_string in a few places to preserve the existing string imprecision. I could have used impl Into<{D,Subd}iagnosticMessage> in various places as is done in the compiler, but that would have required changes to many call sites (mostly changing &format("...") to format!("...")) which didn't seem worthwhile.

r? @WaffleLapkin

rustbot · 2023-05-19T01:40:23Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

The Miri subtree was changed

cc @rust-lang/miri

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

compiler/rustc_builtin_macros/src/compile_error.rs

src/tools/miri/src/diagnostics.rs

WaffleLapkin · 2023-05-19T15:28:30Z

Do I understand it right, that since we need to store diagnostics, having &'not_static str and String are equivalent (because we convert former into latter)?

(i.e. I want to make sure we don't exclude anything by requiring &'static str)

nnethercote · 2023-05-19T22:15:50Z

Do I understand it right, that since we need to store diagnostics, having &'not_static str and String are equivalent (because we convert former into latter)?

I guess you could say that. I.e. currently we convert any &str into a String in the From impl by allocating. This PR means we avoid doing that most of the time when the error message is a &'static str.

bors · 2023-05-20T14:18:52Z

☔ The latest upstream changes (presumably #111778) made this pull request unmergeable. Please resolve the merge conflicts.

nnethercote · 2023-05-23T02:47:51Z

I have rebased and addressed @RalfJung's comment.

src/tools/clippy/clippy_utils/src/diagnostics.rs

src/tools/miri/src/diagnostics.rs

compiler/rustc_hir_analysis/src/check/compare_impl_item.rs

Resolved

nnethercote · 2023-05-25T23:11:47Z

@WaffleLapkin: what's the status here, are we good to proceed?

WaffleLapkin

I'm not entirely sure this is a good idea, since some call cites got marginally more complex, but it's probably fine.

r=me with the nit

compiler/rustc_hir_analysis/src/check/intrinsic.rs

Each of `{D,Subd}iagnosticMessage::{Str,Eager}` has a comment: ``` // FIXME(davidtwco): can a `Cow<'static, str>` be used here? ``` This commit answers that question in the affirmative. It's not the most compelling change ever, but it might be worth merging. This requires changing the `impl<'a> From<&'a str>` impls to `impl From<&'static str>`, which involves a bunch of knock-on changes that require/result in call sites being a little more precise about exactly what kind of string they use to create errors, and not just `&str`. This will result in fewer unnecessary allocations, though this will not have any notable perf effects given that these are error paths. Note that I was lazy within Clippy, using `to_string` in a few places to preserve the existing string imprecision. I could have used `impl Into<{D,Subd}iagnosticMessage>` in various places as is done in the compiler, but that would have required changes to *many* call sites (mostly changing `&format("...")` to `format!("...")`) which didn't seem worthwhile.

nnethercote · 2023-05-28T23:25:03Z

@bors r=WaffleLapkin

bors · 2023-05-28T23:25:04Z

📌 Commit 781111e has been approved by WaffleLapkin

It is now in the queue for this repository.

bors · 2023-05-29T07:10:46Z

⌛ Testing commit 781111e with merge 70e04bd...

bors · 2023-05-29T10:01:44Z

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 70e04bd to master...

rust-timer · 2023-05-29T11:17:41Z

Finished benchmarking commit (70e04bd): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.7%	[0.6%, 0.8%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.8%, -0.3%]	10
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.1%	[2.9%, 6.6%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 642.999s -> 643.87s (0.14%)

nnethercote · 2023-05-30T09:06:26Z

Perf changes are all for secondary benchmarks. The small wins and small losses balance out.

@rustbot label: +perf-regression-triaged

lifetimes for those slices changed to 'static

rustbot assigned WaffleLapkin May 19, 2023

compiler-errors reviewed May 19, 2023

View reviewed changes

compiler/rustc_builtin_macros/src/compile_error.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

nnethercote force-pushed the Cow-DiagnosticMessage branch from 6e4f480 to 6f545b4 Compare May 19, 2023 01:56

This comment has been minimized.

Sign in to view

RalfJung reviewed May 19, 2023

View reviewed changes

src/tools/miri/src/diagnostics.rs Outdated Show resolved Hide resolved

nnethercote force-pushed the Cow-DiagnosticMessage branch from 6f545b4 to c0e83a0 Compare May 23, 2023 02:47

This comment has been minimized.

Sign in to view

nnethercote force-pushed the Cow-DiagnosticMessage branch from c0e83a0 to a146c39 Compare May 23, 2023 03:53

flip1995 previously requested changes May 23, 2023

View reviewed changes

src/tools/clippy/clippy_utils/src/diagnostics.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 23, 2023

View reviewed changes

src/tools/miri/src/diagnostics.rs Outdated Show resolved Hide resolved

WaffleLapkin reviewed May 23, 2023

View reviewed changes

compiler/rustc_hir_analysis/src/check/compare_impl_item.rs Outdated Show resolved Hide resolved

nnethercote force-pushed the Cow-DiagnosticMessage branch from a146c39 to 104f104 Compare May 23, 2023 23:36

WaffleLapkin approved these changes May 26, 2023

View reviewed changes

compiler/rustc_hir_analysis/src/check/intrinsic.rs Show resolved Hide resolved

nnethercote force-pushed the Cow-DiagnosticMessage branch from 104f104 to 781111e Compare May 28, 2023 23:23

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 2023

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 28, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label May 29, 2023

bors merged commit 70e04bd into rust-lang:master May 29, 2023

rustbot added this to the 1.72.0 milestone May 29, 2023

rustbot added the perf-regression Performance regression. label May 29, 2023

nnethercote deleted the Cow-DiagnosticMessage branch May 30, 2023 09:02

rustbot added the perf-regression-triaged The performance regression has been triaged. label May 30, 2023

smoelius added a commit to trailofbits/dylint that referenced this pull request May 31, 2023

Address rust-lang/rust#111748

f92c6a2

smoelius added a commit to trailofbits/dylint that referenced this pull request May 31, 2023

Address rust-lang/rust#111748

f5a13c1

smoelius added a commit to trailofbits/dylint that referenced this pull request May 31, 2023

Address rust-lang/rust#111748

dc49dc1

smoelius added a commit to trailofbits/dylint that referenced this pull request May 31, 2023

Address rust-lang/rust#111748

7270e4f

smoelius added a commit to trailofbits/dylint that referenced this pull request May 31, 2023

Address rust-lang/rust#111748

356bcba

smoelius added a commit to trailofbits/dylint that referenced this pull request Jun 1, 2023

Address rust-lang/rust#111748

77095e6

floriangru added a commit to floriangru/creusot that referenced this pull request Jul 6, 2023

fix rust-lang/rust#111748

56096e4

lifetimes for those slices changed to 'static

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `Cow` in `{D,Subd}iagnosticMessage`. #111748

Use `Cow` in `{D,Subd}iagnosticMessage`. #111748

nnethercote commented May 19, 2023

rustbot commented May 19, 2023

This comment has been minimized.

This comment has been minimized.

WaffleLapkin commented May 19, 2023

nnethercote commented May 19, 2023

bors commented May 20, 2023

nnethercote commented May 23, 2023

This comment has been minimized.

nnethercote commented May 25, 2023

WaffleLapkin left a comment

nnethercote commented May 28, 2023

bors commented May 28, 2023

bors commented May 29, 2023

bors commented May 29, 2023

rust-timer commented May 29, 2023

nnethercote commented May 30, 2023

Use Cow in {D,Subd}iagnosticMessage. #111748

Use Cow in {D,Subd}iagnosticMessage. #111748

Conversation

nnethercote commented May 19, 2023

rustbot commented May 19, 2023

This comment has been minimized.

This comment has been minimized.

WaffleLapkin commented May 19, 2023

nnethercote commented May 19, 2023

bors commented May 20, 2023

nnethercote commented May 23, 2023

This comment has been minimized.

nnethercote commented May 25, 2023

WaffleLapkin left a comment

Choose a reason for hiding this comment

nnethercote commented May 28, 2023

bors commented May 28, 2023

bors commented May 29, 2023

bors commented May 29, 2023

rust-timer commented May 29, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

nnethercote commented May 30, 2023

Use `Cow` in `{D,Subd}iagnosticMessage`. #111748

Use `Cow` in `{D,Subd}iagnosticMessage`. #111748